Support GitHub app auth #14

scottillogical · 2025-03-14T14:51:10Z

The PR enables using GitHub app authentication

The has the following benefits

Using github app auth increases the rate limit vs using a personal authentication token.
Using github app auth allows you to authenticate via the organization as opposed to an individual. So changes to an individual user account will not affect the pipeline (such as changing organization role from owner to member)

Relevant github issue: telia-oss#212
Changes

Add new fields to source for github app auth
add https://github.com/bdellegrazie/git-credential-github-app to dockerfile

bgandon

Thanks for the contribution!
Here are my overall remarks.

Rationale: we'll need to to explain a bit clearer the problem that arises in the first place, and how you solve it with the code you're contributing.
Clean code: could you please rebase your work on top of main branch, and clean it up with git rebase -i so that you remove unnecessary back-and-forth commits, and remove test-related commits?
Failing build: in our Concourse pipeline, the make all build step fails with this error:

> [builder 5/5] RUN go version &&  make all:
...
#14 37.87 vet: ./models_test.go:42:5: unknown field GithubOrganziation in struct literal of type resource.Source
#14 37.97 make: *** [Makefile:11: test] Error 1

Increased code complexity, yet not covered by unit tests: there are many if g.UseGithubApp or if !g.UseGithubApp which increases a lot the number of code branches and thus tests cases that should be covered. Un fortunately we don't have sufficient test coverage to support such complexity.

bgandon · 2025-03-16T15:45:19Z

CODEOWNERS

@@ -1,3 +0,0 @@
-# Default. Unless we have a more specific match.


Please don't remove files unless you have a good reason to do so

restored 52456f6

bgandon · 2025-03-16T15:47:13Z

Dockerfile

-
-FROM ${golang} AS builder
-
+FROM public.ecr.aws/docker/library/golang:1.22 as builder


Please don't change this Dockerfile that is required by the build system in our Concourse pipeline, unless you have good reason to do so.

Instead, you should use a separate Dockerfile to run your tests locally.

Sorry will take a look. I can see that this has been heavily customized for concourse, but as I hope you can understand that makes it seemingly impossible to build locally

bgandon · 2025-03-16T15:47:26Z

Dockerfile

 ENV DEBIAN_FRONTEND=noninteractive
 RUN apt-get -y -qq update \
    && apt-get -y -qq install "make"

 ADD . /go/src/github.com/telia-oss/github-pr-resource
 WORKDIR /go/src/github.com/telia-oss/github-pr-resource

-RUN go version \
-    && make all
+RUN go version &&  make all


No need for such a change

bgandon · 2025-03-16T15:51:09Z

Dockerfile

    openssh \
    git-crypt
 COPY scripts/askpass.sh /usr/local/bin/askpass.sh
+ENV GITHUB_APP_CRED_HELPER_VERSION="v0.3.2"
+ENV BIN_PATH_TARGET=/usr/local/bin
+RUN curl -L https://github.com/bdellegrazie/git-credential-github-app/releases/download/${GITHUB_APP_CRED_HELPER_VERSION}/git-credential-github-app_${GITHUB_APP_CRED_HELPER_VERSION}_Linux_x86_64.tar.gz | tar zxv -C ${BIN_PATH_TARGET}


Adding such external component should be justified in the PR description.

Plus, why not use the latest v0.3.3 version?

Which raises the question of how we shall bump this new dependency?

I updated it to the latest version 9f4ae0f

I am open to suggestions for how to keep it up to date. with say, the git resource, we have to build our own version of the git resource to add support for github app auth, since the regular git resource is agnostic to providers. but supporting github apps in this resource seems highly desirable

Dockerfile

bgandon · 2025-03-16T16:10:11Z

README.md

+| `github_organization`       | No       |  `Vault-tec`                     | Which Github organization your github app is in.
+| `private_key`               | No       |  `-----BEGIN RSA...`             | Private key for your github app.
+| `installation_id`           | No       |  `12356`                         | Installation id for your github app.
+| `application_id`            | No       |  `12356`                         | Application id for your github app.


Table rows should be formatted the same as previous row are.

(We typically use the "format selection" action on the table, using the yzhang.markdown-all-in-one VScode plugin.)

bgandon · 2025-03-16T16:11:30Z

git.go

+	} else {
+		if err := g.command("git", "config", "url.https://[email protected]/.insteadOf", "[email protected]:").Run(); err != nil {
+			return fmt.Errorf("failed to configure github url: %s", err)
+		}


Smaller branch of if-then-else should come first: please invert the condition.

bgandon · 2025-03-16T16:22:39Z

github.go

+		// Client using oauth2 wrapper
+		client = oauth2.NewClient(ctx, oauth2.StaticTokenSource(
+			&oauth2.Token{AccessToken: s.AccessToken},
+		))


Smaller branch of if-then-else should come first: please invert the condition.

This commit adds support for authentication as a GitHub App. This is beneficial in several ways, including: * Increased rate limits * Better separation of access * Finer grained control over access * Removes the need for a bot or service account As part of these changes, have added the `github.com/bradleyfalzon/ghinstallation/v2` module and it's associated dependencies. Added several new configuration fields: * `UseGitHubApp` - Boolean flag signalling if user wants to auth as a GitHub App * `PrivateKeyFile` - Filename for RSA Private Key generated for GitHub App * `AppID` - GitHub App application numerical identifier * `InstallationID` - GitHub App installation numerical identifier Upgrade to golang `1.16` Add some tests for the `Source` model. Add support for both `private_key` and `private_key_file` intputs. This enables either reading the private key from a file, or providing the contents of the private key. Expanded testing to cover additional scenarios. Also rename `AppID` to `ApplicationID`. add manifest yaml bump remove unused travis use real image tags in dockefile, update go.mod update go sum comment xoauth basic comment xoauth basic comment xoauth basic comment xoauth basic add git credentail helper fix dockerfile add credential helper sds sds

remove run e remove run e fix gofmt fix gofmt fix gofmt

remove logger

restore oauth basic remove private key file reference typo punctuation Update Golang from 1.22.10 to 1.23.7 and its dependencies

remove unneeded manifest.yaml

scottillogical · 2025-03-17T18:58:40Z

Thanks for the contribution! Here are my overall remarks.

Rationale: we'll need to to explain a bit clearer the problem that arises in the first place, and how you solve it with the code you're contributing.

Clean code: could you please rebase your work on top of main branch, and clean it up with git rebase -i so that you remove unnecessary back-and-forth commits, and remove test-related commits?

Failing build: in our Concourse pipeline, the make all build step fails with this error:
> [builder 5/5] RUN go version &&  make all:
...
#14 37.87 vet: ./models_test.go:42:5: unknown field GithubOrganziation in struct literal of type resource.Source
#14 37.97 make: *** [Makefile:11: test] Error 1
Increased code complexity, yet not covered by unit tests: there are many if g.UseGithubApp or if !g.UseGithubApp which increases a lot the number of code branches and thus tests cases that should be covered. Un fortunately we don't have sufficient test coverage to support such complexity.

Added "why" to the PR description
Rebased my commits
Fixed build failures
I added some test coverage

bgandon requested changes Mar 16, 2025

View reviewed changes

scottillogical force-pushed the support_github_app_auth branch from 094d02e to 6806207 Compare March 17, 2025 18:46

fatmcgav-depop and others added 9 commits March 17, 2025 14:52

github app support in git clone

e510076

remove run e remove run e fix gofmt fix gofmt fix gofmt

fix github org

a76e2d3

disable access token when using github app

8dc473a

remove logger

add github app docs to readme

4834f59

restore oauth basic remove private key file reference typo punctuation Update Golang from 1.22.10 to 1.23.7 and its dependencies

revert changes to dockerfile - update github app credential helper

ace00a2

invert condition

d53c757

invert condition

79f07a7

formatted selection

1249e3e

scottillogical force-pushed the support_github_app_auth branch from 6806207 to 17a664d Compare March 17, 2025 18:54

fix github organization

f05bdfd

remove unneeded manifest.yaml

scottillogical force-pushed the support_github_app_auth branch from 4d4d7f5 to f05bdfd Compare March 17, 2025 18:56

		@@ -1,3 +0,0 @@
		# Default. Unless we have a more specific match.


		FROM ${golang} AS builder

		FROM public.ecr.aws/docker/library/golang:1.22 as builder

Support GitHub app auth #14

Are you sure you want to change the base?

Support GitHub app auth #14

Uh oh!

Conversation

scottillogical commented Mar 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The PR enables using GitHub app authentication

Uh oh!

bgandon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottillogical Mar 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottillogical commented Mar 17, 2025

Uh oh!

Uh oh!

scottillogical commented Mar 14, 2025 •

edited

Loading

scottillogical Mar 17, 2025 •

edited

Loading